Skip to content

Expose Replace* methods - #2316

Merged
Amir-61 merged 1 commit into
masterfrom
expose_replace_methods
Jun 13, 2016
Merged

Expose Replace* methods#2316
Amir-61 merged 1 commit into
masterfrom
expose_replace_methods

Conversation

@Amir-61

@Amir-61 Amir-61 commented May 9, 2016

Copy link
Copy Markdown
Member
Description:
  • Re-mapping updateAttributes and updateOrCreate endpoints to use PATCH verb by default for 3.X.
  • Exposing replaceById and replaceORCreate via PUT verbs by default for 3.X.
  • Add a flag model.settings.options.updateOnPUT for 3.X.

Note: This patch is for 3.X

Related to #2330 (Implementation for 2.x)

Connect to #1979

@Amir-61 Amir-61 added the #review label May 9, 2016
@Amir-61 Amir-61 changed the title Expose Replace* methods Expose Replace* methods 3.X May 11, 2016
@Amir-61
Amir-61 force-pushed the expose_replace_methods branch from 48607b6 to 20238ca Compare May 11, 2016 17:48
@Amir-61 Amir-61 mentioned this pull request May 12, 2016
1 task
@Amir-61 Amir-61 changed the title Expose Replace* methods 3.X WIP: Expose Replace* methods 3.X May 12, 2016
@Amir-61 Amir-61 assigned Amir-61 and unassigned bajtos May 12, 2016
@Amir-61 Amir-61 changed the title WIP: Expose Replace* methods 3.X WIP: Expose Replace* methods May 12, 2016
@Amir-61
Amir-61 force-pushed the expose_replace_methods branch from 63dec67 to 3d70d65 Compare May 12, 2016 22:07
@Amir-61 Amir-61 assigned bajtos and unassigned Amir-61 May 13, 2016
Comment thread lib/persisted-model.js Outdated

if (options.updateOnPUT) {
configurableVerbs.replaceById = 'post';
configurableVerbs.replaceOrCreate = 'post';

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When updateOnPUT is set, then you need to change the HTTP path too, not only the verb.

updateOnPUT=false

  replaceById     => PUT /:id
  replaceOrCreate => PUT /

updateOnPUT=true

  replaceById     => POST /:id/replaceById
  replaceOrCreate => POST /replaceOrCreate

@bajtos bajtos assigned Amir-61 and unassigned bajtos May 13, 2016
Comment thread lib/persisted-model.js Outdated
});

if (!options.replaceOnPUT) {
setRemoting(PersistedModel, 'upsert', {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's correct to call setRemoting twice for the same method, expecting it to create two HTTP endpoints. The correct way is to provide an array of values in the http property. See how exists defines two HTTP endpoints:

       http: [
         { verb: 'get', path: '/:id/exists' },
         { verb: 'head', path: '/:id' },
       ],

We should do the same here. For example:

var upsertEndpoints = [{ verb: 'patch', path: '/' }];
if (!options.replaceOnPUT) {
  upsertEndpoints.push( {
    verb: 'put',
    patch: '/',
  });
}

setRemoting('PersistedModel', 'upsert', {
  // ...
  http: upsertEndpoints
});

Or you can use the approach you have in place for replaceOrCreateOptions below, both ways are fine.

@Amir-61 Amir-61 closed this May 24, 2016
@Amir-61
Amir-61 force-pushed the expose_replace_methods branch from 81ca747 to 70cec07 Compare May 24, 2016 22:03
@Amir-61 Amir-61 removed the #review label May 24, 2016
@Amir-61 Amir-61 reopened this May 24, 2016
@Amir-61
Amir-61 force-pushed the expose_replace_methods branch 2 times, most recently from 16a0712 to feb5da1 Compare May 24, 2016 22:34
@Amir-61

Amir-61 commented May 25, 2016

Copy link
Copy Markdown
Member Author

Hey @bajtos

Please note strong-remoting does not support re-registering/re-exposing endpoints for models; that means once the models get created, endpoints get registered/exposed so then later on you cannot pass something like the following and expect the new endpoints get registered for store model:

app.models.store.settings.replaceOnPUT = true;
app.models.store.setup();

In other words strong-remoting does not have this capability; the only way you can pass these settings is when you create the models. Hence, I changed the test suits to have two versions of models (let's say for store model); one with replaceOnPUT set to true and one for replaceOnPUT set to false in JSON file fixtures.

This patch is almost done; I would say 90% is done; could you please confirm you agree with my approach?

Thanks Miroslav!

Comment thread lib/persisted-model.js Outdated

// This is just for LB 3.x
if (options.replaceOnPUT !== false || options.replaceOnPUT === undefined) {
options.replaceOnPUT = true;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this could be rewritten as follows?

options.replaceOnPUT = options.replaceOnPUT !== false;

Comment thread test/remoting.integration.js Outdated
});
});

describe('With model.settings.replaceOnPUT true' +

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference between this test and it('has expected remote methods with model.settings.replaceOnPUT set to true' above? I think it's enough to keep only one of these tests, isn't it?

@bajtos

bajtos commented Jun 7, 2016

Copy link
Copy Markdown
Member

@slnode test please

@Amir-61

Amir-61 commented Jun 7, 2016

Copy link
Copy Markdown
Member Author

The CI failures would be passed once this PR#2375 lands. Please see this comment

@bajtos I addressed all of your concerns. Could you PTAL again?

Also I believe still another PR with some changes needs to be submitted for 2.x and a review may be needed for that as well. There would be some changes between these two patches (3.x Vs. 2.x)

Thanks :-)

.reduce(function(p, c) {
return p.concat(c);
});
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also please note I have changed the logic for formatMethod, getFormattedMethodsExcludingRelations, getFormattedScopeMethods,... based on the changes in strong-remoting...

@Amir-61
Amir-61 force-pushed the expose_replace_methods branch from 99b9ada to 27a5eb9 Compare June 7, 2016 22:09
Comment thread test/model.test.js Outdated
'removeById',
'count',
'prototype.updateAttributes',
'prototype.patchAttributes', 'updateAttributes',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the second item/alias should have prototype. prefix too.

 'prototype.patchAttributes', 'prototype.updateAttributes'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we either need to fix L627 to prepend prototype. prefix to isStatic:false methods, or change remoting definition of prototype.patchAttributes to specify method alias as prototype.updateAttributes. This depends on how strong-remoting treats aliases of prototype methods, which is something I am not sure about. Could you please check? It is also worth checking how loopback-connector-remote handles these aliases, to ensure code calling myModelInstance.updateAttributes keeps working (assuming the model is attached to a remote-backed datasource).

@bajtos bajtos removed their assignment Jun 10, 2016
*Re-mapping `updateAttributes` endpoint to use
`PATCH` and `PUT`(configurable) verb
*Exposing `replaceById` and `replaceOrCreate` via
`POST` and `PUT`(configurable) verb
@Amir-61
Amir-61 force-pushed the expose_replace_methods branch from a9426db to 6502309 Compare June 10, 2016 18:59
@Amir-61

Amir-61 commented Jun 10, 2016

Copy link
Copy Markdown
Member Author

@bajtos I addressed your feedback... Please review :-)

PS: CI failures do not seem relevant.

@Amir-61

Amir-61 commented Jun 11, 2016

Copy link
Copy Markdown
Member Author

@slnode test please

1 similar comment
@Amir-61

Amir-61 commented Jun 13, 2016

Copy link
Copy Markdown
Member Author

@slnode test please

@bajtos

bajtos commented Jun 13, 2016

Copy link
Copy Markdown
Member

LGTM 👏

@bajtos bajtos removed their assignment Jun 13, 2016
@Amir-61
Amir-61 merged commit 6bbe7be into master Jun 13, 2016
@Amir-61 Amir-61 removed the #review label Jun 13, 2016
@Amir-61 Amir-61 mentioned this pull request Aug 4, 2016
@Amir-61
Amir-61 deleted the expose_replace_methods branch September 30, 2016 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants